Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Add function to run tests more consistently #87

Closed
wants to merge 2 commits into from

Conversation

TravisEz13
Copy link
Member

@TravisEz13 TravisEz13 commented Feb 5, 2017

@PlagueHO
Copy link
Contributor

PlagueHO commented Feb 5, 2017

Hi @TravisEz13:

This looks like it is replicating much of the functionality in PR #85.

How will this work with resources that use a test harness (e.g. SharePointDsc and xNetworking - as well as xSQLServer soon)?

This PR does seem to be in conflict with much of discussion that has been happening in these PR's:
PowerShell/DscResources#246
PowerShell/DscResources#180
PowerShell/DscResources#205
PowerShell/DscResources#244
dsccommunity/NetworkingDsc#177

I'd suggest that this is looked at in conjunction with the HQRM modules SharePointDsc and xNetworking before it is merged 😢 Sorry to throw my oar in!


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


TestRunner.psm1, line 137 at r1 (raw file):

#>
function Push-TestArtifact

I don't think this Module is the correct place for this function. This should be added to AppVeyor.psm1 as it is an AppVeyor only cmdlet. AppVeyor.psm1 was added in #85


Comments from Reviewable

@TravisEz13
Copy link
Member Author

Reviewing what has been done in the SharePointDsc... I think this idea of a test harness is counter to the idea of HQRM. We want to have a common, reliable test harness. If functionality is required above and beyond Pester that is not specific to the module (reviewing SharePoint's harness most of it is not,) it should be added to a common module.

We should not have different harnesses in each resource.

AppVeyor.psm1 is not currently part of the code. I think the module, as is, is too small to be factored into multiple modules. I was planning the refactoring in later updates.

I disagree reviewing the discussion is most of the threads. I think the creation of test harnesses was due to the lack of such functionality in the central repro.

@TravisEz13
Copy link
Member Author

I filed #88 to give you an idea of all the work involved in this project.

@TravisEz13
Copy link
Member Author

reviewing #85 it looks like it might have achieved the first goal in #88. Let's discuss.

@PlagueHO
Copy link
Contributor

PlagueHO commented Feb 5, 2017

What I guess I'm hoping for is some consistent plan or instruction for sharing all the common tests and code between all the different repos that works for all repositories (including SharePointDsc and xNetworking). I really don't like code replication and there is a huge amount of it across the tests, AppVeyor and deployment code - my PR was intended to help address that.

There is so much tech debt being introduced every time the test/deploy process is changed that if we reduce the amount of code replication in the test/deploy code across the resources then we reduce the amount of tech debt being added. This means that adding "code coverage" or additional common test types cost us all less in the long run. So I do think that it would be much more cost effective (tech debt wise) to add the shared test/deploy code before adding code coverage changes.

Plus making these test/deploy processes consistent makes it so much easier to maintain.

I'm not totally sure why the Test Harness was used by SharePointDsc (@BrianFarnhill could you clarify?), but @tysonjhayes has copied this over to xNetworking. xSQLServer and xStorage are both also being moved over to this as well. If the test harness method is going to be in conflict with what you're doing then shouldn't this be addressed before making this change?

Anyway, I completely agree with your goals in #88. What I'm hoping is that it can be made to work for xNetworking, SharePointDsc, xSQLServer and xStorage as well as combined with the work I've been doing unifying the common tests and AppVeyor code.

I'll leave this discussion to the rest of the community as I've got a lot of other stuff on right now and am happy to go with whatever you all think is best (you're the experts 😁)

@TravisEz13
Copy link
Member Author

#90 replaces this PR

@TravisEz13 TravisEz13 closed this Feb 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants